Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DASH] Make DASH vnet orch bulk logic consistent #3554

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

theasianpianist
Copy link
Contributor

What I did

  • Make pre/post bulk methods in DashVnetOrch have consistent return value meaning

Why I did it
Fixes #3349

How I verified it

Details if related

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>
@mssonicbld
Copy link
Collaborator

/azp run

@theasianpianist theasianpianist requested review from prabhataravind and Yakiv-Huryk and removed request for prsunny March 10, 2025 23:22
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@theasianpianist theasianpianist requested a review from prsunny March 10, 2025 23:24
@@ -430,7 +430,7 @@ bool DashVnetOrch::addVnetMap(const string& key, VnetMapBulkContext& ctxt)
SWSS_LOG_INFO("Not creating VNET map for %s since VNET %s doesn't exist", key.c_str(), ctxt.vnet_name.c_str());
return false;
}
return addOutboundCaToPa(key, ctxt) && addPaValidation(key, ctxt);
return addOutboundCaToPa(key, ctxt) || addPaValidation(key, ctxt);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should still be "AND" and not "OR".

If let's say addPaValidation returns true and addOutboundCaToPa returns false, we don't want the entry to be removed from the consumer.

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

Signed-off-by: Lawrence Lee <lawlee@microsoft.com>
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[dash] VNET mapping configuration is not processed correctly
3 participants